Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix RTS_GMLC_RT_sys horizon #102

Merged
merged 2 commits into from
Jul 8, 2024
Merged

Fix RTS_GMLC_RT_sys horizon #102

merged 2 commits into from
Jul 8, 2024

Conversation

GabrielKS
Copy link
Collaborator

In PSY3, a time series' horizon was specified as a multiple of its resolution; in PSY4 it's its own TimePeriod. Within build_RTS_GMLC_RT_sys, the horizon of 24 was incorrectly migrated to 24 hours rather than to 24*(5 minute resolution) = 2 hours.

This may be my smallest ever pull request. Several hours of head scratching went into that one deleted character….

The PSY3 horizon was 24, here that was incorrectly interpreted as 24
hours rather than as 24*(5 minute resolution) = 2 hours
@daniel-thom
Copy link
Contributor

This was my bad...thanks for fixing. Did you look through the other cases already? If not, I can do that.

@GabrielKS
Copy link
Collaborator Author

This was my bad...thanks for fixing. Did you look through the other cases already? If not, I can do that.

A brief search only revealed one other case in PSB where we were specifying the horizon manually this way and there the resolution happened to be an hour so it worked out. It'd be great if you could double-check.

Copy link

codecov bot commented Jul 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.29%. Comparing base (517429a) to head (8eb3993).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #102   +/-   ##
=======================================
  Coverage   94.29%   94.29%           
=======================================
  Files          15       15           
  Lines        3312     3312           
=======================================
  Hits         3123     3123           
  Misses        189      189           
Flag Coverage Δ
unittests 94.29% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
src/library/psi_library.jl 98.37% <100.00%> (ø)

@daniel-thom
Copy link
Contributor

This was my bad...thanks for fixing. Did you look through the other cases already? If not, I can do that.

A brief search only revealed one other case in PSB where we were specifying the horizon manually this way and there the resolution happened to be an hour so it worked out. It'd be great if you could double-check.

There is one other change in https://github.com/NREL-Sienna/PowerSystemCaseBuilder.jl/pull/90/files that looks suspicious. @jd-lara You should probably re-review that PR.

@jd-lara
Copy link
Member

jd-lara commented Jul 3, 2024

This was my bad...thanks for fixing. Did you look through the other cases already? If not, I can do that.

A brief search only revealed one other case in PSB where we were specifying the horizon manually this way and there the resolution happened to be an hour so it worked out. It'd be great if you could double-check.

There is one other change in https://github.com/NREL-Sienna/PowerSystemCaseBuilder.jl/pull/90/files that looks suspicious. @jd-lara You should probably re-review that PR.

#90 (comment)

@GabrielKS GabrielKS requested a review from jd-lara July 3, 2024 20:39
@jd-lara jd-lara merged commit 123c49e into main Jul 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants